Skip to content

Conversation

@Jean-Beru
Copy link
Contributor

In order to get the next node when traversing a tree, this PRs add new methods NestedTreeRepository::getNextNode and NestedTreeRepository::getNextNodes. They take some arguments :

  • $root is the top node and is used to limit result to this (sub-)tree
  • $node is the current node (or null for the first call)
  • $limit (only for getNextNodes) can limit nodes count to return
  • $traversalStrategy accepts self::TRAVERSAL_PRE_ORDER (default) or self::TRAVERSAL_LEVEL_ORDER and is used to determine which algorithm use (see https://www.geeksforgeeks.org/tree-traversals-inorder-preorder-and-postorder).

These methods can be used, for example, to read a book page after page:

  • The History of Middle-earth
    • The Book of Lost Tales 1
      • Chapter 1
        • Page 1
        • Page 2
        • Page 3
        • ...
      • Chapter 2
        • Page 1
        • Page 2
        • Page 3
        • ...
    • The Book of Lost Tales 2
    • ...

@Jean-Beru Jean-Beru force-pushed the feat/get-next-node branch from 2fad25b to 442d99b Compare July 27, 2022 08:22
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.30%. Comparing base (f21d888) to head (0037f83).
⚠️ Report is 183 commits behind head on main.

Files with missing lines Patch % Lines
...rc/Tree/Entity/Repository/NestedTreeRepository.php 95.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2498      +/-   ##
==========================================
+ Coverage   79.22%   79.30%   +0.07%     
==========================================
  Files         161      161              
  Lines        8415     8455      +40     
==========================================
+ Hits         6667     6705      +38     
- Misses       1748     1750       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@morban
Copy link

morban commented Apr 25, 2023

It will be nice if this PR was successful and merge. Thank you.

@Jean-Beru Jean-Beru force-pushed the feat/get-next-node branch from 42c0094 to 079cec2 Compare July 28, 2023 08:35
@Jean-Beru Jean-Beru requested a review from phansys July 28, 2023 08:43
@phansys phansys requested a review from franmomu August 1, 2023 09:34
Copy link
Collaborator

@franmomu franmomu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can add object type to parameters


### Added
- Tree: Add `Nested::ALLOWED_NODE_POSITIONS` constant in order to expose the available node positions
- Tree: Add pre-order and level-order traversals (#2498). Add `getNextNode()` and `getNextNodes()` methods to traverse the tree
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to the ## [Unreleased] section

*
* @throws InvalidArgumentException if input is invalid
*/
public function getNextNodesQueryBuilder($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodesQueryBuilder($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder
public function getNextNodesQueryBuilder(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder

Should we make this method private?

* @param int|null $limit Maximum nodes to return. If null, all nodes will be returned
* @param self::TRAVERSAL_* $traversalStrategy Strategy to use to traverse tree
*/
public function getNextNodesQuery($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodesQuery($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query
public function getNextNodesQuery(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query

* @param object|null $node Current node. If null, first node will be returned
* @param self::TRAVERSAL_* $traversalStrategy Strategy to use to traverse tree
*/
public function getNextNode($root, $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNode($root, $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object
public function getNextNode(object $root, ?object $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object

*
* @return array<object>
*/
public function getNextNodes($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodes($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array
public function getNextNodes(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array

@Jean-Beru
Copy link
Contributor Author

@franmomu I agree to add the object type to arguments and make some functions private. Since this logic could be applied to other NestedTreeRepository functions (get*QueryBuilder and get*Query), should we:

  • keep as it is to have the same approach
  • make only new typehinted and private
  • make all typehinted and private => will cause a BC break
    ?

IMO, the second one it the best answer since we improve code without any BC break. Before doing any commit, is it OK for you @phansys ?

}
} elseif (self::TRAVERSAL_LEVEL_ORDER === $traversalStrategy) {
if (!isset($config['level'])) {
throw new \InvalidArgumentException('TreeLevel must be set to use level order traversal.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why \InvalidArgumentException? $config['level'] isn't provided as an argument in this method.

@phansys
Copy link
Collaborator

phansys commented Aug 12, 2023

IMO, the second one it the best answer since we improve code without any BC break. Before doing any commit, is it OK for you @phansys ?

LGTM.

@phansys phansys self-requested a review August 18, 2023 00:58
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels Feb 14, 2024
@Jean-Beru
Copy link
Contributor Author

github-actions, you can keep this PR opened. I'll fix conflicts soon.

@franmomu franmomu added the Still Relevant Mark PRs that might've been auto-closed that are still relevant. label Jun 9, 2024
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels Feb 14, 2025
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Rebase New Feature Still Relevant Mark PRs that might've been auto-closed that are still relevant. Tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants